Skip to content

feat: add ulims authz policy#310

Open
MattPrit wants to merge 3 commits into
mainfrom
feat/mp/add-ulims
Open

feat: add ulims authz policy#310
MattPrit wants to merge 3 commits into
mainfrom
feat/mp/add-ulims

Conversation

@MattPrit
Copy link
Copy Markdown
Collaborator

@MattPrit MattPrit commented May 26, 2026

Summary: Adds authZ policy for use in ULIMS services. Essentially, this moves the existing ULIMS policy into the central repository, with the addition of service account support and a new rule for filtering a list of instruments

Added rules:

  • allow - top-level check that the user has been verified
  • session_restrictions - returns the sessions that the user has access to, in the format [{"beamline": "i03", "proposal_number": 1, "visit_number": 1}, ...]
  • filter_sessions - given an input instrument_sessions of (proposal number, session number) pairs, return those pairs corresponding to session that the user has access to
  • filter_instruments - given an in put instruments, a list of instrument names, return those that the user has access to (i.e is an admin of)

Note: There is a section of policy that I have copied from tiled.rego

Note: Some of the rules depend on token.claims.beamline, I suspect that more generally we would like token.claims.instruments, but I have used beamline for consistency with tiled.rego

@MattPrit MattPrit marked this pull request as ready for review May 27, 2026 10:40
@MattPrit MattPrit force-pushed the feat/mp/add-ulims branch from eb36e68 to 6e60748 Compare May 27, 2026 10:41
@MattPrit MattPrit requested a review from ZohebShaikh May 27, 2026 11:02
Copy link
Copy Markdown
Collaborator

@ZohebShaikh ZohebShaikh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the docs that you have put for the policy, We should add a regal lint for this if possible in a separate PR.

Note: Some of the rules depend on token.claims.beamline, I suspect that more generally we would like token.claims.instruments, but I have used beamline for consistency with tiled.rego

I agree we should make the change from beamline to instrument. The faster we do this the better as it will have lesser impact. This needs to be changed from the bundler as well

Comment on lines +9 to +63
# --- Start section copied from tiled.rego ---

subject := data.diamond.data.subjects[token.claims.fedid]

# METADATA
# title: Beamlines
# description: |
# Identifies all beamlines the subject is authorized to access
# based on their assigned permissions.
beamlines contains beamline if {
token.claims.fedid
not admin.is_admin(token.claims.fedid)
some p in subject.permissions
some beamline in object.get(data.diamond.data.admin, p, [])
}

# Aggregates all session IDs the subject is authorized to view.
# Admins receive a wildcard "*" granting access to all sessions.
# Regular users gain session access through three pathways:
# 1. Direct session membership
# 2. Access via beamline-level permissions
# 3. Access via proposal-level permissions
user_sessions contains "*" if {
subject
admin.is_admin(token.claims.fedid)
}

user_sessions contains format_int(session, 10) if {
subject
not admin.is_admin(token.claims.fedid)
some session in subject.sessions
}

user_sessions contains format_int(session, 10) if {
subject
not admin.is_admin(token.claims.fedid)
some beamline in beamlines
some session in data.diamond.data.beamlines[beamline].sessions
}

user_sessions contains format_int(session, 10) if {
subject
not admin.is_admin(token.claims.fedid)
some p in subject.proposals
some i in data.diamond.data.proposals[format_int(p, 10)]
some session in i
}

# service account check
user_sessions contains format_int(session, 10) if {
not subject
some session in data.diamond.data.beamlines[token.claims.beamline].sessions
}

# --- End section copied from tiled.rego ---
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should move this to session because it looks like a common thing rather than application specific.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beamlines as well, or is that better placed in a separate file?

# entrypoint: true
default session_restrictions := []

session_restrictions := null if {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally don't like the "null" restrictions, But I understand what you are trying to do here


allow if {
token.verified[0]
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can move this to token.rego as valid_token True/False, looks like common policy to me.

Do you make call to OPA to verify your token?
We do this locally in blueapi like this

Comment on lines +110 to +115
filter_sessions contains session if {
some session in input.instrument_sessions
proposal_number := format_int(session[0], 10)
session_number := format_int(session[1], 10)
format_int(data.diamond.data.proposals[proposal_number].sessions[session_number], 10) in user_sessions
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
filter_sessions contains session if {
some session in input.instrument_sessions
proposal_number := format_int(session[0], 10)
session_number := format_int(session[1], 10)
format_int(data.diamond.data.proposals[proposal_number].sessions[session_number], 10) in user_sessions
}
filter_sessions contains session if {
"*" not in user_sessions
some session in input.instrument_sessions
proposal_number := format_int(session[0], 10)
session_number := format_int(session[1], 10)
format_int(data.diamond.data.proposals[proposal_number].sessions[session_number], 10) in user_sessions
}

# - `input.instruments`, an array of strings representing a list of instruments
# entrypoint: true
filter_instruments contains instrument if {
some instrument in input.instruments
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not token.claims.beamline

}

filter_instruments contains instrument if {
admin.is_admin(token.claims.fedid)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not token.claims.beamline


filter_instruments contains instrument if {
admin.is_admin(token.claims.fedid)
some instrument in input.instruments
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not check if the instrument is a valid instrument?

If a admin type input.instrument = ["area-51-beamline"] it should return []

filter_instruments contains instrument if {
token.claims.beamline
some instrument in input.instruments
instrument == token.claims.beamline
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here it should verify the beamline is valid

@MattPrit
Copy link
Copy Markdown
Collaborator Author

MattPrit commented Jun 1, 2026

I agree we should make the change from beamline to instrument. The faster we do this the better as it will have lesser impact...

What about multi-valued claims (i.e. multiple instrument access)? Should I move to token.claims.instrument and assume a single value for now?

@ZohebShaikh
Copy link
Copy Markdown
Collaborator

What about multi-valued claims (i.e. multiple instrument access)? Should I move to token.claims.instrument and assume a single value for now?
I'm little reluctant to add instruments because I feel like the UDC client should be 1 per beamline and I have not come across a use case where 1 beamline needs access to 2 or more beamlines data.
So I think we should leave it to a single value for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants